Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix AnsiOutput write methods claiming zero bytes were written #31

Merged
merged 1 commit into from
Sep 16, 2018

Conversation

hawkw
Copy link
Contributor

@hawkw hawkw commented Sep 16, 2018

Currently, the implementation of IStdout for AnsiOutputalways returns Ok(0) when a write completes successfully.

This behaviour is incorrect, as these methods are used elsewhere to implement std::io::Write, which expects the writer to return the number of bytes written. This means that the implementation of Write for crossterm::Terminal doesn't play nice with the write! macro (and other consumers of the Write trait), causing uses of write! to panic with the message "failed to write whole buffer". (see mgattozzi/ysh#12).

This branch changes these methods to correctly return the number of bytes written, fixing the panics.

Copy link
Contributor

@AnneKitsune AnneKitsune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

write!(handle, "{}", string)?;
handle.flush();
Ok(0)
let out = &self.handle;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason there are 5 spaces instead of 4 now.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, the line 16 has only 3 spaces so it looks like 5.

Copy link
Member

@TimonPost TimonPost Sep 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing! You are right jojolepro I see that all functions are one space outlined. I'll clean it up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that, the original function was missing one space of indentation, so I tried to fix it, but I think I missed line 16. My bad.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry already fixed it! But thanks for your pull request!

@TimonPost
Copy link
Member

Thanks, my fault :( ! I'll merge it an do a release.

I have on question could the release wait for a couple of days? This because there maybe come another PR so I could have two patches in one release. If that doesn't work out let me know!

You could then use the GitHub repo with master branch in your cargo.toml

@TimonPost TimonPost merged commit 0eba0e2 into crossterm-rs:master Sep 16, 2018
@TimonPost
Copy link
Member

TimonPost commented Sep 22, 2018

I published the new version 4.2 with the changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants